-
Notifications
You must be signed in to change notification settings - Fork 348
Convert execd to use pcmk__request_t #3915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1b3051b
to
6b0ff95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed through CRM_OP_REGISTER
commit. May or may not get further tonight.
daemons/execd/execd_messages.c
Outdated
|
||
if (!c->name) { | ||
const char *value = crm_element_value(msg, | ||
PCMK__XA_LRMD_CLIENTNAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why the attribute would ever be missing, so I
think it's safe to get rid of the lengthier approach that grabs the
client PID as a last option.
I don't think c->name
gets set anywhere outside of this function and lrmd_remote_client_msg()
.
PCMK__XA_LRMD_CLIENTNAME
gets set when an API client connects. (And it strangely gets re-set to the value of c->name
here and in lrmd_remote_client_msg()
, which seems redundant.)
lrmd_t:connect()
-> lrmd_handshake()
-> lrmd_handshake_hello_msg()
But the lrmd API is public, and the name
argument to lrmd_t:connect()
can be NULL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checks clock Good night.
{ | ||
int call_id = 0; | ||
int rc = pcmk_rc_ok; | ||
bool allowed = pcmk_is_set(request->ipc_client->flags, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might eventually be nice to do something like we have with cib__op_attr_privileged
, to reduce some duplication in the handlers. Maybe also a flag for needing call ID, then based on that, getting it and CRM_CHECK
-ing that we succeeded; or something. Not worth the trouble right now.
See also enum crm_rsc_flags
.
I've addressed most review comments, but there's still the ack/nack and response return code stuff to straighten out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update looks good to me, aside from the handful of comments that I just left (e.g., CRM_LOG_ASSERT()
). I should've made them into a review instead of single comments for ease of finding, oh well...
As you mentioned, there are still things to consider before merging. I'm pretty much good with the parts that have been addressed though.
This is the beginnings of using the pcmk__request_t interface in the server side of pacemaker-execd. For now, there's no substantial code changes - just moving the code into a new file.
This brings them more in line with usage in pe_ipc_dispatch (I'm hoping to condense all this code one day), and frees up the name "request" to be used by pcmk__request_t in the future.
At the moment, this function is fairly unfortunately named (there's already a process_lrmd_message). However, the new function is gradually going to be taking code from the old function until the old one is completely gone. So it's just a temporary confusion we'll need to live with for a bit. This new function holds the framework for handling commands with pcmk__request_t instead of the old system. It's a little tricky to convert these commands over one at a time and still keep everything working, which is why there's the big TODO comment.
Both places that call execd_process_message first add several additional attributes to the request XML, so it makes sense to move that code into execd_process_message instead. Note that there are two different approaches to setting client->name if it's missing. I don't see why the attribute would ever be missing, so I think it's safe to get rid of the lengthier approach that grabs the client PID as a last option.
This commit is typical of how the commit for each command is going to work: * Expose a function in execd_commands.c to the rest of the exec daemon and give it a name that is consistent with "public" functions. * Have it return a standard pacemaker return code. * Remove the old handler block from process_lrmd_message (this is the older function, not the newer confusingly named function). * Add a new handler function and add that function to the handler table.
...and rename them to make them consistent. These are needed to handle processing certain message types.
Handling this message type means we also need to send out a generic notify message after sending the reply. For the moment, the rc value being passed to execd_send_generic_notify doesn't matter because execd_process_rsc_register doesn't return anything (previously, it only ever returned pcmk_ok).
Additionally, execd_process_rsc_exec should return a standard pacemaker return code. It doesn't do anything useful with the call_id value it extracts and returns, and the caller already knows the call_id (and doesn't do anything with the return value from execd_process_rsc_exec aside from check if it's an error or not).
This additionally changes a couple functions to return a standard Pacemaker return code.
This requires a little extra work, in addition to the steps of exposing a function and changing its return type to be a standard Pacemaker return code. Sending a generic notify for unregistering a resource also requires inspecting the return value, which we luckily have in the reply XML so we can just grab it from there as needed.
Now that all messages are supported by the new code, we can add the unknown message handler to catch anything else. And that means everything is being handled by execd_process_message now, so process_lrmd_message can be removed. Fixes T126
and rename the existing function to be execd_create_reply_as. This gets rid of the need to pass __func__ as the first argument every time.
Only a couple daemons send an ACK on an unknown request. Those that do send CRM_EX_INVALID_PARAM, which doesn't quite seem like the right value. I guess I can see what it's going for, but CRM_EX_PROTOCOL seems like a better fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me, except see ACK/NACK comment.
daemons/attrd/attrd_ipc.c
Outdated
@@ -596,7 +596,7 @@ attrd_ipc_dispatch(qb_ipcs_connection_t * c, void *data, size_t size) | |||
|
|||
if (xml == NULL) { | |||
crm_debug("Unrecognizable IPC data from PID %d", pcmk__client_pid(c)); | |||
pcmk__ipc_send_ack(client, id, flags, PCMK__XE_ACK, NULL, | |||
pcmk__ipc_send_ack(client, id, flags, PCMK__XE_NACK, NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, but am not remotely sure, that some clients may need to be updated -- either to return early, or to properly handle a NACK in some other way.
Previously, the only things that sent NACK were the CIB manager (for invalid message), the fencer, and the executor (in remote_proxy_cb()
).
No client looks for NACK, but they do look for ACK. The following clients' IPC dispatch functions return immediately if they receive an ACK (but not a NACK):
- attrd
- controld
- pacemakerd
- schedulerd
I'm starting to think it'd be simpler to drop PCMK__XE_NACK
and drop the tag
argument of pcmk__ipc_send_ack()
, and instead send PCMK__XE_ACK
unconditionally. Clients can use the exit code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's kind of what I was thinking too based on the commit message. I don't remember why we have both ACK vs. NACK and an exit code. One other thing to note here is that not every message gets an ACK, either. I'm also not convinced that any of this matters too terribly much either given that we control both sides of the connection and it takes place on the same system with what I assume will always be the same versions of the pacemaker daemons.
I'd be fine with dropping this patch, merging this PR, and then dealing with the ACK vs. NACK stuff in a later PR.
We have a long-standing project to convert all daemons to use the
pcmk__request_t
type. Once this is done, we can start reducing code duplication across the daemons and start doing a bunch more cleanups (see projects related to T126). This PR addresses execd.I've done this a little bit weird in order to make it easier to develop. Since there's a bunch of different IPC messages in execd and it's easier to develop and debug if you only convert them one at a time, but we also have a default handler, I've added a commit titled "XXX DON'T PUSH THIS" and then a later commit that reverts it. These two get rid of the default handler, allowing the old code to still be used as a fallback for messages I haven't converted yet.
Obviously, I would get rid of these before pushing this PR for real. But, doing that will also make it difficult to bisect through this PR should we ever need to. So, I'm open to suggestions.
I haven't given this the thorough testing it requires yet due to the problem I mentioned in slack where sometimes resources don't start, but in less comprehensive testing, this has been working fine.